@W-20481777 Setup Generic Telemetry interface #190
Conversation
| try { | ||
| // Select provider based on configuration | ||
| switch (config.telemetry.provider) { | ||
| case 'moncloud': |
There was a problem hiding this comment.
i think having separate cases for moncloud vs custom will become even more apparent when i need to write custom metrics for the tools using telemetry.addAttributes(), as each underlying telemetry provider has different apis for doing that
| /** | ||
| * Configuration for telemetry providers | ||
| */ | ||
| export interface TelemetryConfig { |
There was a problem hiding this comment.
Consider enforcing the requirement at the type level that providerConfig must be provided when provider === "custom" with something like:
export type TelemetryConfig = {
enabled: boolean;
} & (
| {
provider: "noop" | "moncloud";
}
| {
provider: "custom";
providerConfig: Record<string, unknown>;
}
);
|
|
||
| this.telemetry = { | ||
| enabled: telemetryEnabled === 'true', | ||
| provider: (telemetryProvider as 'noop' | 'moncloud' | 'custom') || 'noop', |
There was a problem hiding this comment.
Consider using a type guard for the telemtryProvider, similar to how this.auth is set below. Currently the runtime value could be anything.
Something like:
const telemetryProviders = ["noop", "moncloud", "custom"] as const;
type TelemetryProvider = (typeof telemetryProviders)[number];
export function isTelemetryProvider(value: unknown): value is TelemetryProvider {
return !!(telemetryProviders.find((t) => t === value));
}
...
provider: isTelemetryProvider(telemetryProvider) ? telemetryProvider : 'noop',|
|
||
| case 'noop': | ||
| default: | ||
| if (config.telemetry.provider !== 'noop') { |
There was a problem hiding this comment.
The type of config.telemetry.provider and the return type on this method prevent this default case from ever happening. If you remove the default case and, say, add a new provider type without adding the new case for the new provider, TypeScript will yell at you because the method could potentially now return undefined
| } | ||
|
|
||
| // Instantiate the provider with the full config | ||
| return new ProviderClass(config); |
There was a problem hiding this comment.
Before returning the instance, let's verify the class implements the interface. I did something similar here: https://github.com/tableau/tableau-mcp/blob/anyoung/session-store/src/server/storage/storeFactory.ts#L157
| import { TelemetryAttributes, TelemetryProvider } from './types.js'; | ||
|
|
||
| export class MonCloudTelemetryProvider implements TelemetryProvider { | ||
| private trace: any; |
There was a problem hiding this comment.
What sets trace? And can it have a better type?
IMPORTANT: Please do not create a Pull Request without creating an issue first.
Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of
the pull request.
Pull Request Template
Description
Creating a generic interface for plugging in the salesforce telemetry agent OR a custom telemetry provider.
Motivation and Context
Type of Change
How Has This Been Tested?
Related Issues
Checklist
npm run version. For example,use
npm run version:patchfor a patch version bump.environment variable or changing its default value.
Contributor Agreement
By submitting this pull request, I confirm that:
its Contribution Checklist.